Skip to content

Add --exit-on-failure flag to runbms for CI use#169

Merged
caizixian merged 10 commits intomasterfrom
copilot/fix-161
Sep 29, 2025
Merged

Add --exit-on-failure flag to runbms for CI use#169
caizixian merged 10 commits intomasterfrom
copilot/fix-161

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Jul 27, 2025

  • Identify merge conflicts with master branch containing --randomize-configs feature
  • Resolve conflicts by integrating both --exit-on-failure and --randomize-configs features
  • Update documentation to include both flags in usage syntax and descriptions
  • Merge both feature tests in test suite
  • Clean up any remaining conflict markers
  • Verify tests pass with both features working together
  • Apply black code formatting as required by CI

Implementation Details

Successfully resolved merge conflicts with the master branch that added the --randomize-configs flag while preserving all functionality of the --exit-on-failure flag. Both features now work together:

  • --exit-on-failure [CODE]: Exits with specified code (default: 1) when any configuration fails
  • --randomize-configs: Randomizes config order for each invocation to distinguish system vs config-specific issues

The merge integrated:

  • Combined import statements (both sys and random modules)
  • Merged argument parser setup for both flags
  • Unified global variable initialization
  • Preserved all exit logic for failure scenarios
  • Combined test suites covering both features
  • Updated documentation with both flags in usage syntax and descriptions

All tests pass and code formatting applied successfully.


💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

Co-authored-by: caizixian <2891235+caizixian@users.noreply.github.com>
Copilot AI changed the title [WIP] Support exit code 1 for CI use Add --exit-on-failure flag to runbms for CI use Jul 27, 2025
Copilot AI requested a review from caizixian July 27, 2025 03:31
@caizixian
Copy link
Copy Markdown
Member

@copilot Currently you wait for all invocations within one hfac to finish before checking whether any of them has fail. Check for failure and exit immediately after execution.

Pass --no-pager to all git commands (note that --no-pager is an argument to git not git subcommands) to prevent CLI waiting for user input.

Co-authored-by: caizixian <2891235+caizixian@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Jul 27, 2025

@copilot Currently you wait for all invocations within one hfac to finish before checking whether any of them has fail. Check for failure and exit immediately after execution.

Pass --no-pager to...

I've implemented immediate exit behavior for the --exit-on-failure flag. The tool now exits with code 1 immediately after any configuration fails, rather than waiting for all invocations within one hfac to complete. The change adds sys.exit(1) calls to all 6 failure scenarios in the run_one_benchmark function. Commit: 5037041

@caizixian
Copy link
Copy Markdown
Member

@copilot Notice that any_config_failed global flag is no longer needed. Please clean up the code accordingly.

See the GitHub CI configs in the repo for how to run various tests, including running black to format the code before committing.

Pass --no-pager to all git commands (note that --no-pager is an argument to git not git subcommands) to prevent CLI waiting for user input.

Co-authored-by: caizixian <2891235+caizixian@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Jul 27, 2025

@copilot Notice that any_config_failed global flag is no longer needed. Please clean up the code accordingly.

See the GitHub CI configs in the repo for how to run various tests, including running...

You're absolutely right. Since the code now exits immediately on failure when the flag is enabled, the any_config_failed global variable was redundant. I've cleaned up the code by removing:

  • The any_config_failed global variable declaration
  • All 6 assignments to any_config_failed = True in failure scenarios
  • The end-of-run checks that used any_config_failed for deferred exit
  • Updated tests accordingly and applied black formatting

The logic is now much cleaner - immediate exit on failure when flag is enabled, always exit 0 otherwise. Commit: daf0f96

@caizixian
Copy link
Copy Markdown
Member

@copilot ignore this new flag for dry run. Leave the original code as is.

Co-authored-by: caizixian <2891235+caizixian@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Jul 27, 2025

@copilot ignore this new flag for dry run. Leave the original code as is.

I've updated the code to ignore the --exit-on-failure flag for dry run mode. In dry-run mode, the original behavior is preserved - it always exits with code 0 regardless of the flag setting. The change removes the exit logic for SubprocessrExit.Dryrun cases, restoring the original behavior of just printing "." and continuing. Commit: 0d89116

@caizixian
Copy link
Copy Markdown
Member

@copilot Remove test_exit_on_failure_immediate_exit test. This is useless. Also update the changelog for this PR under unreleased.

Co-authored-by: caizixian <2891235+caizixian@users.noreply.github.com>
@caizixian caizixian requested a review from qinsoon July 27, 2025 04:08
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Jul 27, 2025

@copilot Remove test_exit_on_failure_immediate_exit test. This is useless. Also update the changelog for this PR under unreleased.

I've removed the test_exit_on_failure_immediate_exit test and updated the changelog under the "Unreleased" section to document the new --exit-on-failure flag. Applied black formatting as well. Commit: a033260

@caizixian caizixian marked this pull request as ready for review July 27, 2025 04:10
@caizixian
Copy link
Copy Markdown
Member

@copilot Instead of exit 1, change the flag to take an optional argument to allow user to specify the exit code, but default to 1. Update the changelog, and reference documentation as needed.

…exit code

Co-authored-by: caizixian <2891235+caizixian@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Jul 27, 2025

@copilot Instead of exit 1, change the flag to take an optional argument to allow user to specify the exit code, but default to 1. Update the changelog, and reference documentation as needed.

I've updated the --exit-on-failure flag to accept an optional argument for specifying custom exit codes. The flag now supports:

  • --exit-on-failure (defaults to exit code 1)
  • --exit-on-failure 42 (exits with code 42)
  • No flag (original behavior, exits with code 0)

Updated the documentation, changelog, and tests accordingly. Applied black formatting as well. Commit: 7dcb0df

@caizixian
Copy link
Copy Markdown
Member

@copilot resolve conflicts

Copilot AI and others added 2 commits September 29, 2025 06:27
…figs features

Co-authored-by: caizixian <2891235+caizixian@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Sep 29, 2025

@copilot resolve conflicts

Resolved merge conflicts with master branch. The --exit-on-failure and --randomize-configs features now work together seamlessly. Both flags are properly integrated in the command-line parser, documentation, and test suite. All tests pass and code formatting applied. Commit: 6c33f4b

@caizixian caizixian merged commit a76abfe into master Sep 29, 2025
21 checks passed
@caizixian caizixian deleted the copilot/fix-161 branch September 29, 2025 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants